Checked commit imtayadeway@c696161 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
40 files checked, 0 offenses detected
Everything looks fine. 🍰

who knew 2 small code changes causing 38 spec files changes. time to get ☕️

In general, I'm ok here.

A couple of questions:

1. can a helper method in the rpsec be introduced so we don't have such tedious changes in the future for similar id related changes.
2. should the href returned also reflect the cid, I know this is not an issue as the href is already a string but I wonder if it would be any better for being consistent with the new id returned.

Just thoughts, you can throw 🍅 at it.

@abellotti I'm going to answer these in reverse order, because it will be slightly easier that way ;)

2. I think that is a really good idea, and that we definitely should do that. I think it might be good though to ship that in another PR, especially since I'm anticipating that being a big diff too.

1. I have many, many thoughts about this one, so I'll try my best to enumerate them here, in no particular order:

1. This PR was tedious to complete, but not for that reason! Most of these changes could be made though regex substitutions, so that wasn't hard. The tedious part was finding that things passed locally (where I am running in region 0) and failed in CI (region 1), so I kept having to go back and forth. The reason for that is that region 0 ids, though strings, behave more like ints with some APIs, but regions 1+ do not. I started a discussion in gitter detailing some of these woes
2. To me, it seems natural that many tests would have to be updated, because this is a big change (even though it only involved changing a few lines of production code)
3. Trying to make it easier to do next time I think might be time poorly spent. I know it is impossible to predict the future, but if we were finding that we were regularly changing this I would think we would have serious problems. We'd also have to predict the way things might change - something engineers are traditionally not very good at ;)
4. In general, I am OK with things like custom matchers if they make tests easier to read and don't contain any complex logic (which itself needs to be tested, and usually isn't). I'm generally 👎 on helper methods that remove the specifics from the example body - I want to see those specifics in the body precisely because it is an example
5. I agree that the fact that I had to update a lot of expectations was a problem. But the way I wanted to address that was to start by questioning, what is the value of all these expectations? Is it possible they could have questionable, or even negative value? They have to be updated and maintained, just like production code. Honestly I wanted to delete some of them, because the cost of maintaining them seemed way higher than any value they might add. Tests are often seen as free, but here we see the cost of adding all these expectations when something changes.
6. It seems, for instance, that in a lot of tests we are doing things like fetching the id from the response, then going deep into the internals to see if things really happened. I think expectations like that belong at a lower level test - in integration tests we should be concerned with the request and response only. It it is seen as absolutely necessary, I think it is generally better to use the change matcher - that way we don't need to deal with ids at all.
7. In other places we are putting expectations that the id of a response is being rendered as expected (this is the bulk of the changes). This to me signals that we have poor confidence that rendering ids works, so we test it every time we make a request. Making it easier to change to me isn't DRY - the DRY thing would be to isolate the part of the code responsible for serializing resources, test it thoroughly with all known edge cases, then ensure that new examples only test what is new or unique to it. This the motivation behind much of the refactoring work I am doing.

All that said, I was trying my best to make this PR as unopinionated as possible by simply updating the tests to make them pass (for the sake of the reviewer). But I'd love to address some of these things in a follow up. LMK what you think!

Again, this is ok with me.

We do need to revisit the API rspecs to possibly concentrate on structural matchers, i.e. Action results, queries, tagging, etc. this would simplify/reduce size of our tests yet continue to provide sufficient confident that our consumers get the expected responses. Followup discussions/PRs.

Thanks @imtayadeway for fixing this.